Skip to content

Refactored as a directive #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jun 12, 2016
Merged

Refactored as a directive #6

merged 11 commits into from
Jun 12, 2016

Conversation

ThomasPe
Copy link

@ThomasPe ThomasPe commented Jun 9, 2016

Finally, I got around to dealing with the rebase. Hopefully I've done it right this time - the GitHub Desktop app is really helpful, though.

I have now refactored the builder controller so it can be used as a directive.
I feel it's now at a point where we can start working on it "issues-based" to weed out bugs and add missing features.

@nicklasb
Copy link
Member

nicklasb commented Jun 9, 2016

Cool, I will have a look tomorrow!

@nicklasb
Copy link
Member

nicklasb commented Jun 10, 2016

So, my problem is still that while yes, it is now a directive, the application that housed the functionality is gone. The application was first and foremost actually sort of usable, and second, it made it easy to play around and test the functionality.

It is likely that we should more clearly split those two in some future, but at this stage at least I want the application to remain somewhere in the repository, perhaps as an example or utility, but one that uses the directive. Because as it is now, the sfbSchema isn't really separate anyway, it is in the app structure.

I would propose a build/lib for the directive and its dependencies, and a build/example or build/utility for the.
Do you agree with that? If so I am for merging this to the directive branch now and then continue with re-adding the application.

@rowino : However, you were considering some rewriting, was that WRT the app or the functionality in the directive, the actual builder?

@ThomasPe
Copy link
Author

Sure, that sounds good.
However there's currently one issue - the app is currently storing the form & schema info in one combined file and only splits this into seperate schema and form markup files on export. So with the directive you currently can't edit existing forms.
It certainly would be great to hear @rowino 's thoughts on how to proceed.

@rowino
Copy link
Contributor

rowino commented Jun 10, 2016

@ThomasPe, first thanks for the input. Nice having others contribute into it. As for the directive, I had mentioned to @nicklasb earlier that I was thinking of spliting the generator into a plain js library then create a angular-directive separate. Reason being to allow reuse with other frameworks. We'll then have a site somewhere for people to use it for quick forms while for in-app use one can use the directive or their framework of choice. Does this shade some light?

@nicklasb
Copy link
Member

nicklasb commented Jun 10, 2016

@rowino, @ThomasPe :
That would obviously be great, an ES6 library would make it portable on all web frontends, much as we do now with ASF itself.

Basically that would mean that this repo would become home to the pure ES6 library, and then we would create another for the angular/react/whatever directives/implementations.

The plan for the json-schema-core has been to later call it json-schema-core-es6, as there are other core implementations on the horizon, like json-schema-core-java, json-schema-core-php and whatnot.

Should we therefore rename this repo json-schema-form-builder-es6?
And then implement the directives in angular-schema-form-builder repos and so on?

And then the app, wherever that ends up will be what it wants to be?

@nicklasb
Copy link
Member

Either way, that means that we just merge this and works from there?

@ThomasPe
Copy link
Author

sounds good @rowino & @nicklasb

@nicklasb nicklasb merged commit be91456 into json-schema-form:directive Jun 12, 2016
@nicklasb
Copy link
Member

Done.

@nicklasb nicklasb mentioned this pull request Jun 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants